Core: Fix numeric overflow of timestamp nano literal #11775
Conversation
ebyhr
force-pushed
the
ebi/timestamp-ns
branch
from
5228ca6 to
efe1d14
Compare
| @@ -300,8 +300,7 @@ public <T> Literal<T> to(Type type) { | |||
| case TIMESTAMP: | |||
| return (Literal<T>) new TimestampLiteral(value()); | |||
| case TIMESTAMP_NANO: | |||
| // assume micros and convert to nanos to match the behavior in the timestamp case above | |||
| return new TimestampLiteral(value()).to(type); | |||
| return (Literal<T>) new TimestampNanoLiteral(value()); | |||
There was a problem hiding this comment.
This change seems correct to me, the previous behavior for this was to assume the value was in microseconds and then pass that through to TimestampLiteral but that can overflow and does not actually represent a nanosecond timestamp!
There was a problem hiding this comment.
FWIW I still think this is correct but it's worth getting other's perspective on this since we are changing one of the assumptions of how value is interpreted when the type to convert to is nanoseconds.
CC @nastra @epgif @jacobmarble @rdblue thoughts?
| assertThat(Literal.of(400000L).to(TimestampNanoType.withoutZone()).toByteBuffer().array()) | ||
| .isEqualTo(new byte[] {0, -124, -41, 23, 0, 0, 0, 0}); | ||
| .isEqualTo(new byte[] {-128, 26, 6, 0, 0, 0, 0, 0}); | ||
| assertThat(Literal.of(400000L).to(TimestampNanoType.withZone()).toByteBuffer().array()) |
There was a problem hiding this comment.
I'm a bit confused how the original assertion was passing? Shouldn't have this always been equivalent to {-128, 26, 6, 0, 0, 0, 0, 0}?
There was a problem hiding this comment.
I think the cause is the original logic called DateTimeUtil.microsToNanos method which multiples the value by 1000:
iceberg/api/src/main/java/org/apache/iceberg/expressions/Literals.java
Lines 444 to 445 in b9b61b1
There was a problem hiding this comment.
Ah I see the comment on line 107/108. Could we update the assertConversion to instead test against 400000L and then remove the comment. At this point we are no longer having to pass in different values since we Literal.of(someLong).to(TimestampNanos) will always interpret someLong as nanoseconds.
api/src/test/java/org/apache/iceberg/expressions/TestTimestampLiteralConversions.java
Show resolved
Hide resolved
The previous logic leads to overflow when we pass timestamp nanos long value.